This repeats the earlier claim that JsonExtensionData "must" use Dictionary<string, JsonElement> and that Dictionary<string, object> is invalid. System.Text.Json supports extension data on IDictionary<string, object> as well; if the goal is to prevent accidentally keeping JToken, rephrase this pitfall to focus on avoiding Newtonsoft types and explain the tradeoffs between object and JsonElement/JsonNode.

This eval is being added under src/dotnet/tests/..., but the repo layout and evaluation workflow expect scenarios under tests/<plugin>/<skill>/eval.yaml and only trigger on tests/** (and plugins/**). Placing this under src/ means CI evaluation and CODEOWNERS folder validation won't run for the new scenario. Consider moving to tests/dotnet/migrating-newtonsoft-to-system-text-json/eval.yaml.

DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull is not Newtonsoft.Json's default (Json.NET includes nulls unless configured). Since this method is labeled "Match Newtonsoft.Json default behavior", consider separating true defaults from optional/common settings (or adjust the wording) so the guidance isn't misleading.

The rubric's casing note is inconsistent with the skill doc: it expects a warning about "PascalCase in STJ vs camelCase in Newtonsoft". If you want to test casing differences, consider aligning this with the skill's own statements and/or explicitly scoping it to ASP.NET Core defaults vs library defaults to avoid grading correct answers as wrong.

expect_tools: ["bash"] forces the model to make at least one bash tool call or the scenario fails. This prompt is primarily a code-migration explanation and doesn't require shell usage, so this constraint may introduce unnecessary flakiness. Consider removing expect_tools (or only using it when the scenario includes a setup/fixture that actually needs bash).

The polymorphism section implies System.Text.Json will automatically emit a discriminator for [JsonDerivedType] types. STJ polymorphism requires explicit opt-in/configuration (e.g., JsonPolymorphic / JsonPolymorphismOptions / type-info resolver setup). Consider updating this snippet to show the required opt-in so readers don't assume the attributes alone are sufficient.

These files are being added under src/dotnet/..., but the repo's documented layout and CI evaluation workflows expect skills under plugins/<plugin>/skills/<skill>/SKILL.md and scenarios under tests/<plugin>/<skill>/eval.yaml. As-is, the evaluation workflow (and CODEOWNERS folder validation) won't run because it only triggers on plugins/** and tests/**. Consider moving this skill to plugins/dotnet/skills/migrating-newtonsoft-to-system-text-json/ and the eval to tests/dotnet/migrating-newtonsoft-to-system-text-json/.

The doc contradicts itself on default property naming: Step 1 says both Newtonsoft.Json and System.Text.Json default to PascalCase/as-declared, but Step 2 config sets PropertyNamingPolicy = CamelCase and labels it "Newtonsoft default". Please clarify the distinction (Json.NET library defaults vs ASP.NET Core defaults vs any custom ContractResolver/NamingPolicy) and make the guidance consistent.

[JsonExtensionData] doesn't have to be Dictionary<string, JsonElement> only—System.Text.Json also supports extension data on other shapes (e.g., IDictionary<string, object> and, in newer TFMs, JsonNode/JsonObject variants). If the intent is to recommend JsonElement as the closest replacement for JToken, consider rephrasing this as guidance rather than a hard requirement so the doc stays technically correct.

This example configures JsonSerializerSettings with TypeNameHandling = TypeNameHandling.Auto, which is a known unsafe pattern when used with untrusted JSON because it lets an attacker control the concrete .NET type to instantiate, enabling gadget-based code execution or data tampering. Even though it is labeled as a security risk, including this as compilable example code may lead readers to copy it into production; consider replacing it with a non-usable anti-pattern illustration or a safer configuration that avoids TypeNameHandling for untrusted data.

Setting Preserve significantly increases the attack surface of JSON deserialization. Most call sites do not know how to properly reason about the increased attack surface and could find themselves in a vulnerable state. This is why S.T.J does not support $ref / $id in its default configuration.

It's fine if people want to set this, but they should do it only if they truly need it and have thought through the consequences of setting it. It shouldn't be promoted as a "try it and see if it solves your problem" mechanism. (What happens in practice is that somebody will set it, find that it doesn't solve their problem, then move on to some other diagnostic step without reverting this change.)

To defend against this attack, the application should consider what happens if the adversary controls the edges in the object graph, not just the values of the nodes in the graph. For example, if you're deserializing a graph that represents a family tree, what happens if a node sets itself as its own parent? What happens if a node has two children, but both point to the same object in memory? (This last example isn't recursive; it's an example of a graph that has a many:1 relationship between edges and nodes.)

Both examples could violate business logic that the application otherwise tries to uphold. Allowing an untrusted client to control the edges in the graph represents a broader attack surface and involves more subtle reasoning compared to an untrusted client that can only control values within the graph.

Similarly, this is a potentially dangerous setting and should not be encouraged unless the application has thought through the consequences. We chose not to allow this by default in S.T.J because it could lead to desynced deserialization attacks.

In these attacks, the frontend and the backend disagree on the contents of the payload. This could be because they disagree on where comments start or end, potentially allowing sensitive values to be smuggled through what appears to be an ignorable comment. (For example, Newtonsoft.Json, JSON5, and S.T.J all have different concepts on what "end of line" means for a single-line comment.)

To defend against this attack, the application should:

1. ensure that all deserializers in the system are operating in strict RFC compliance mode with the most restrictive settings (e.g., disallowing duplicate property entries in the payload); or
2. ensure that all components involved in request handling use the same deserializer library, with the same major version and patch level, to ensure consistent payload processing; or
3. ensure the frontend deserializes the payload to an object graph, then reserializes that object graph to a new payload before sending it on to the backend, so the backend sees only what the frontend believed the request to contain.

I would add that case insensitive property handling (recommended a few lines above this) similarly makes systems susceptible to interoperability attacks.

While I get that such settings might be necessary to preserve a modicum of compatibility with Json.NET, at a bare minimum we should try to document security concerns in the skill. It should start off assuming the highest bar possible (using JsonSerializerDefaults.Strict) and then instruct the agent to interview the user about particular requirements that force the loosening of individual settings, e.g. "Should the serializer support comments? Should the serializer allow case insensitive property binding? Should it escape non-ASCII characters? Should it allow trailing commas? Should it allow duplicate JSON properties? etc.)

I think just always recommend JsonNode and friends. It's closer than JsonDocument/JsonElement.

STJ also escapes non-ASCII characters by default. Should recommend using relaxed encoder

I think we should caveat any such recommendation with the trade-offs: escaped JSON strings are semantically equivalent to unescaped ones, so this should depend on how the output is being used. Will it be embedded in HTML? Is it fed to LLMs that might get confused by the escaping? etc.

I'm pretty sure newer models can figure out regexes like that on their, so I would just skip that part for fear of it going out of sync.

Remove this file

Update these to the appropriate team.